admin: Support /drain_listeners?graceful #11639

auni53 · 2020-06-18T14:27:59Z

Signed-off-by: Auni Ahsan [email protected]

Calling /drain_listeners?graceful will trigger the drain manager drain sequence prior to closing listeners.

Risk Level: Low.
Testing: Tested that connections are terminated on request complete during the graceful drain period, that new connections can still be opened, H1/H2-specific response behaviour.
Docs Changes: Add docs to admin.rst, improve the overall drain sequence documentation.

Signed-off-by: Auni Ahsan <[email protected]>

auni53 · 2020-06-18T16:07:27Z

We'd discussed automatically graceful draining on StopListeners::All, but that actually complicates the testing because it removes the ability to force drain in the StopListeners::All case. I'd prefer to just keep this simple for now rather than adding another query parameter that overrides it. (The current testing strategy is to invoke a long graceful drain period, do the testing, then invoke a non-graceful drain to close out.)
What docs/api notes are needed for query parameter changes?

mattklein123 · 2020-06-22T03:06:12Z

This this makes sense to me. You will want to add docs here:
https://www.envoyproxy.io/docs/envoy/latest/operations/admin
https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/operations/draining

/wait

Signed-off-by: Auni Ahsan <[email protected]>

auni53 · 2020-06-22T16:29:50Z

Added docs, not sure if more detail is needed. I think the drain docs probably need an overhaul, but I don't really have the context with other parts of it to want to take that on.

Signed-off-by: Auni Ahsan <[email protected]>

mattklein123

LGTM with small comments.

/wait

mattklein123 · 2020-06-22T21:09:51Z

docs/root/operations/admin.rst

+   .. http:post:: /drain_listeners?graceful
+
+   When draining listeners, enter a graceful drain period prior to closing listeners. 
+   This behaviour and duration is determined by server options for the drain manager, and 


Please ref link to the relevant config and CLI options.

I couldn't figure out how to ref link to the server options, not sure if the ServerInfo proto is what you mean, but I linked the CLI options.

mattklein123 · 2020-06-22T21:11:02Z

docs/root/intro/arch_overview/operations/draining.rst

@@ -12,6 +12,8 @@ various events. Draining occurs at the following times:
 * The server is being :ref:`hot restarted <arch_overview_hot_restart>`.
 * Individual listeners are being modified or removed via :ref:`LDS
  <arch_overview_dynamic_config_lds>`.
+* The server begins shutdown sequence via the :ref:`drain_listeners?graceful


Line 39 below needs updating. Please take a general pass through this page. You have as good an understanding of this as any, and we expect contributors to care for the docs and improve them as they make changes. Thanks!

My bad, I'm kind of rushing for a deadline and writing docs in markdown was a little confusing. Reorganized a lot of this doc and added more detail.

Signed-off-by: Auni Ahsan <[email protected]>

mattklein123

Thanks LGTM with small comments.

/wait

mattklein123 · 2020-06-23T17:14:57Z

docs/root/intro/arch_overview/operations/draining.rst

+In a few different scenarios, Envoy will attempt to gracefully shed connections. For instance,
+during server shutdown, existing requests can be discouraged and listeners set to stop accepting,
+to reduce the number of open connections when the server shuts down. Draining behaviour is defined
+by the server options in addition to indvidiaul listener configs.


typo indvidiaul

mattklein123 · 2020-06-23T17:15:29Z

docs/root/intro/arch_overview/operations/draining.rst

 * Individual listeners are being modified or removed via :ref:`LDS
  <arch_overview_dynamic_config_lds>`.

+By default, the Envoy server will close listeners immediately on server shutdown. To close listeners


Suggested change

By default, the Envoy server will close listeners immediately on server shutdown. To close listeners

By default, the Envoy server will close listeners immediately on server shutdown. To drain listeners

mattklein123 · 2020-06-23T17:18:39Z

docs/root/intro/arch_overview/operations/draining.rst

+:ref:`Redis <config_network_filters_redis_proxy>`, and
+:ref:`Mongo <config_network_filters_mongo_proxy>`.
+:ref:`HTTP connection manager <config_http_conn_man>`,


Mixed up commas, period, etc.

Done I think. markdown is hard to read.

mattklein123 · 2020-06-23T17:19:29Z

source/server/admin/listeners_handler.cc

+
+  const bool graceful = params.find("graceful") != params.end();
+  if (graceful) {
+    server_.drainManager().startDrainSequence([this, stop_listeners_type]() {


If this is called twice does something bad happen? Does this need to be guarded?

Oooh good catch. Added a test, added a guard, left a comment because I'm not sure what the admin response should return if you've called /drain_listeners?graceful twice. Should it silently do nothing? (Since the server's already draining...) Or return an error on the admin call? (If so what error?)

Signed-off-by: Auni Ahsan <[email protected]>

auni53

Outstanding item on what to return from the admin endpoint in case of double calls to startDrainSequence()

auni53 · 2020-06-23T17:25:59Z

docs/root/intro/arch_overview/operations/draining.rst

+In a few different scenarios, Envoy will attempt to gracefully shed connections. For instance,
+during server shutdown, existing requests can be discouraged and listeners set to stop accepting,
+to reduce the number of open connections when the server shuts down. Draining behaviour is defined
+by the server options in addition to indvidiaul listener configs.


auni53 · 2020-06-23T17:26:14Z

docs/root/intro/arch_overview/operations/draining.rst

 * Individual listeners are being modified or removed via :ref:`LDS
  <arch_overview_dynamic_config_lds>`.

+By default, the Envoy server will close listeners immediately on server shutdown. To close listeners


auni53 · 2020-06-23T17:26:27Z

docs/root/intro/arch_overview/operations/draining.rst

+:ref:`Redis <config_network_filters_redis_proxy>`, and
+:ref:`Mongo <config_network_filters_mongo_proxy>`.
+:ref:`HTTP connection manager <config_http_conn_man>`,


Done I think. markdown is hard to read.

auni53 · 2020-06-23T17:29:22Z

source/server/admin/listeners_handler.cc

+
+  const bool graceful = params.find("graceful") != params.end();
+  if (graceful) {
+    server_.drainManager().startDrainSequence([this, stop_listeners_type]() {


Oooh good catch. Added a test, added a guard, left a comment because I'm not sure what the admin response should return if you've called /drain_listeners?graceful twice. Should it silently do nothing? (Since the server's already draining...) Or return an error on the admin call? (If so what error?)

mattklein123 · 2020-06-23T17:41:18Z

source/server/admin/listeners_handler.cc

+
+  const bool graceful = params.find("graceful") != params.end();
+  if (graceful) {
+    if (server_.drainManager().drainClose()) {


Is this always going to return true depending on the settings? I think you need an actual "drain sequence started" boolean?

In terms of what to do, I don't feel strongly about it as long as it's documented well. I think returning 200 OK is fine with me.

/wait

Yeah actually this is going to flakily break on probabilistic drain. The underlying draining_ variable already exists, do you think I should just add a getter to the drain manager interface? Hopefully won't need to change the parent DrainDecision class.

Yeah I think a getter on the base interface is fine which would just say whether the drain sequence has started.

Signed-off-by: Auni Ahsan <[email protected]>

mattklein123

LGTM other than missing comment, thanks.

/wait

mattklein123 · 2020-06-23T19:38:16Z

include/envoy/server/drain_manager.h

@@ -21,6 +21,8 @@ class DrainManager : public Network::DrainDecision {
   */
  virtual void startDrainSequence(std::function<void()> drain_complete_cb) PURE;

+  virtual bool draining() const PURE;


Add doc comments for the interface method.

Done, thanks for the quick turnaround.

Signed-off-by: Auni Ahsan <[email protected]>

mattklein123

Thanks!

htuch · 2020-06-24T13:44:39Z

Mac OS X CI failure seemed unrelated (looks like it was just hanging, CC @asraa as on-call).

Calling /drain_listeners?graceful will trigger the drain manager drain sequence prior to closing listeners. Risk Level: Low. Testing: Tested that connections are terminated on request complete during the graceful drain period, that new connections can still be opened, H1/H2-specific response behaviour. Docs Changes: Add docs to admin.rst, improve the overall drain sequence documentation. Signed-off-by: Auni Ahsan <[email protected]>

Calling /drain_listeners?graceful will trigger the drain manager drain sequence prior to closing listeners. Risk Level: Low. Testing: Tested that connections are terminated on request complete during the graceful drain period, that new connections can still be opened, H1/H2-specific response behaviour. Docs Changes: Add docs to admin.rst, improve the overall drain sequence documentation. Signed-off-by: Auni Ahsan <[email protected]> Signed-off-by: yashwant121 <[email protected]>

Add ?graceful query parameter to /drain_close

4c6f8ce

Signed-off-by: Auni Ahsan <[email protected]>

htuch requested a review from mattklein123 June 18, 2020 15:57

htuch assigned mattklein123 Jun 18, 2020

auni53 changed the title ~~admin: dd ?graceful query parameter to /drain_close~~ admin: Support /drain_close?graceful Jun 18, 2020

auni53 marked this pull request as ready for review June 19, 2020 15:18

repokitteh-read-only bot added the waiting label Jun 22, 2020

add docs

e569e57

Signed-off-by: Auni Ahsan <[email protected]>

repokitteh-read-only bot removed the waiting label Jun 22, 2020

auni53 added 3 commits June 22, 2020 19:11

fix spacing?

4b98fd2

Signed-off-by: Auni Ahsan <[email protected]>

fix reference

b9c32ed

Signed-off-by: Auni Ahsan <[email protected]>

add newline

d830849

Signed-off-by: Auni Ahsan <[email protected]>

mattklein123 requested changes Jun 22, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 22, 2020

Add to docs

648b121

Signed-off-by: Auni Ahsan <[email protected]>

repokitteh-read-only bot removed the waiting label Jun 23, 2020

mattklein123 requested changes Jun 23, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 23, 2020

auni53 added 2 commits June 23, 2020 17:29

Doc fixes

7b4db20

Signed-off-by: Auni Ahsan <[email protected]>

Support double drain

403a78d

Signed-off-by: Auni Ahsan <[email protected]>

repokitteh-read-only bot removed the waiting label Jun 23, 2020

auni53 changed the title ~~admin: Support /drain_close?graceful~~ admin: Support /drain_listeners?graceful Jun 23, 2020

Fix formatting

55c013d

Signed-off-by: Auni Ahsan <[email protected]>

auni53 commented Jun 23, 2020

View reviewed changes

mattklein123 reviewed Jun 23, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 23, 2020

auni53 added 2 commits June 23, 2020 18:13

add draining()

b7405df

Signed-off-by: Auni Ahsan <[email protected]>

expect 200, change strategy

0164050

Signed-off-by: Auni Ahsan <[email protected]>

repokitteh-read-only bot removed the waiting label Jun 23, 2020

mattklein123 requested changes Jun 23, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 23, 2020

add class interface note

5b6d83b

Signed-off-by: Auni Ahsan <[email protected]>

repokitteh-read-only bot removed the waiting label Jun 23, 2020

mattklein123 approved these changes Jun 23, 2020

View reviewed changes

htuch merged commit 77efb47 into envoyproxy:master Jun 24, 2020

auni53 deleted the graceful branch June 24, 2020 13:45

twghu mentioned this pull request Mar 16, 2021

Load balance long-lived connections upon auto scale-out or scale-in of Envoy #15283

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

admin: Support /drain_listeners?graceful #11639

admin: Support /drain_listeners?graceful #11639

auni53 commented Jun 18, 2020 •

edited

Loading

auni53 commented Jun 18, 2020

mattklein123 commented Jun 22, 2020

auni53 commented Jun 22, 2020

mattklein123 left a comment

mattklein123 Jun 22, 2020

auni53 Jun 23, 2020

mattklein123 Jun 22, 2020

auni53 Jun 23, 2020

mattklein123 left a comment

mattklein123 Jun 23, 2020

auni53 Jun 23, 2020

mattklein123 Jun 23, 2020

auni53 Jun 23, 2020

mattklein123 Jun 23, 2020

auni53 Jun 23, 2020

mattklein123 Jun 23, 2020

auni53 Jun 23, 2020

auni53 left a comment

auni53 Jun 23, 2020

auni53 Jun 23, 2020

auni53 Jun 23, 2020

auni53 Jun 23, 2020

mattklein123 Jun 23, 2020

auni53 Jun 23, 2020

mattklein123 Jun 23, 2020

auni53 Jun 23, 2020

mattklein123 left a comment

mattklein123 Jun 23, 2020

auni53 Jun 23, 2020

mattklein123 left a comment

htuch commented Jun 24, 2020

	By default, the Envoy server will close listeners immediately on server shutdown. To close listeners
	By default, the Envoy server will close listeners immediately on server shutdown. To drain listeners

admin: Support /drain_listeners?graceful #11639

admin: Support /drain_listeners?graceful #11639

Conversation

auni53 commented Jun 18, 2020 • edited Loading

auni53 commented Jun 18, 2020

mattklein123 commented Jun 22, 2020

auni53 commented Jun 22, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auni53 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

htuch commented Jun 24, 2020

auni53 commented Jun 18, 2020 •

edited

Loading